Update MSRVs to be accurate #6742

itsjunetime · 2024-11-17T01:06:41Z

Which issue does this PR close?

What changes are included in this PR?

This changes MSRVs of a lot of crates in this repo to accurately reflect their actual MSRV, as they've fallen out of date.

There's an MSRV check in CI, but it currently only checks a few specific crates instead of all of them. It also doesn't have any way to tell if the workspace's rust-version field has fallen out of date due to all other crates in the workspace using a specifically different MSRV. This fixes both of those issues by checking every Cargo.toml that exists in this repo and also making sure at least one of them uses rust-version = { workspace = true }.

The MSRVs that these were changed to were all found by running cargo msrv find --manifest-path $cargo_toml_path for each Cargo.toml in the repo. If it failed due to a dependency that we could downgrade, I downgraded it as low as our Cargo.toml allowed it to go to see if that would allow us to get the MSRV lower.

Rationale for this change

The repo is easier to use and manage if this task is automated

Also, at time of writing, the arrow-flight/gen crate has a broken MSRV. This is to ensure the CI checks that I added actually work. I'll change it back once CI has verified it's bad.

Are there any user-facing changes?

No; these crates already wouldn't compile with their current (incorrect) MSRVs so we aren't affecting users at all with these changes.

… few and purposefully break arrow-flight msrv

…where

itsjunetime · 2024-11-17T01:07:21Z

.github/workflows/rust.yml

@@ -122,24 +122,20 @@ jobs:
        uses: ./.github/actions/setup-builder
      - name: Install cargo-msrv
        run: cargo install cargo-msrv
-      - name: Downgrade arrow dependencies
-        run: cargo update -p ahash --precise 0.8.7


ahash's MSRV now is 1.60, so we don't need this downgrade anymore.

itsjunetime · 2024-11-17T01:22:35Z

The MSRV check failed, and although the output was too long to see what exactly the error was, I'm going to update the purposefully-broken package's MSRV to see if things pass now.

…pace as a whole

alamb

Thank you so much @itsjunetime for cleaning this up 🙏

The basic pattern looks very promising to me

The CI is currently failing https://github.com/apache/arrow-rs/actions/runs/11876059759/job/33093978528?pr=6742

I ran the check locally

cargo msrv verify --manifest-path object_store/Cargo.toml

And the actual appears to be this

  │ error: package `tokio v1.39.1` cannot be built because it requires rustc 1.70 or newer, while the currently active rustc version is 1.64.0 │

alamb · 2024-11-17T10:47:44Z

.github/workflows/rust.yml

-      - name: Check object_store
-        working-directory: object_store
-        run: cargo msrv --log-target stdout verify
+      - name: Check all packages


This is awesome ❤️

alamb · 2024-11-17T10:50:25Z

.github/workflows/rust.yml

+            echo "Checking package '$dir'"
+            cargo msrv verify --manifest-path "$dir" || exit 1
+          done
+      # If no packages are using the workspace's rust-version, then it's out of date


Given that cargo msrv can't currently check a crate that uses the workspace MSRV, maybe we should just remove the workspace MSRV in favor of explicit versions in all the crates 🤔

alamb · 2024-11-22T20:17:22Z

I took the liberty of pushing some commits to this branch to get this PR unstuck and hopefully ready to merge

Updated object_store stated msrv
removed workspace check
merged from apache/main

tustvold · 2024-11-22T22:38:00Z

object_store/Cargo.toml

@@ -24,7 +24,7 @@ readme = "README.md"
 description = "A generic object store interface for uniformly interacting with AWS S3, Google Cloud Storage, Azure Blob Storage and local files."
 keywords = ["object", "storage", "cloud"]
 repository = "https://github.com/apache/arrow-rs/tree/master/object_store"
-rust-version = "1.64.0"
+rust-version = "1.70.0"


I'm surprised by this given we had CI checking this previously?

I double checked, and indeed you are right that cargo msrv still passes with 1.64 (but it requires downgrading tokio)

Not sure what is going on. Pushed a change to revert

@alamb it seems you changed this to 1.70 in bfe798d from what I had set it to (1.62.1, what cargo msrv find reports after downgrading tokio). I'm not quite sure why you did that - unless there's a reason this must be 1.64, I think it would be best to drop it down to 1.62.1.

I agree -- I think it was my mistake to push it to 1.64. Let's put it back to 1.62.1

tustvold · 2024-11-29T17:31:23Z

I struggle to approve this as a number of these crates were being tested with MSRV and were passing (e.g. arrow-flight)... Am I missing something?

still not sure about this

itsjunetime · 2024-12-02T16:23:07Z

I struggle to approve this as a number of these crates were being tested with MSRV and were passing (e.g. arrow-flight)... Am I missing something?

Yes - everything was passing before, but it simply wasn't testing every crate in this repo and we also had multiple MSRVs set too high (which msrv testing wouldn't catch, since we don't try to verify that the MSRV is set as low as it can go)

For example, arrow-flight was passing before, and we didn't change anything about it - however, the arrow-flight/gen crate had the MSRV set too low. It was set to 1.70.0, when the lowest version it actually supported was 1.71.1. We didn't catch this because the arrow-flight crate was already set to 1.71.1, and that was the only thing we were testing.

In the same vein, the arrow-integration-testing and arrow-pyarrow-integration-testing weren't being checked by CI at all either, so their MSRVs had fallen out of sync. This probably won't impact dependents at all, as I doubt anyone is using these crates besides this repo alone, but it's just nice to have them more up-to-date with this change now.

Besides these sorts of things, the other main change is updating the workspace's MSRV to 1.70 (from 1.62) since it had just fallen out-of-date with what the minimum MSRV is within the workspace. Since the workspace MSRV is now 1.70, all the crates that previously had their MSRV explicitly set to "1.70.0" can now just use { workspace = true }.

This PR fixes the above issues by:

Using a find loop to test every Cargo.toml in the repo for CI, instead of just a few hand-picked specific ones
Making sure at least one crate in the repo uses rust-version = { workspace = true } so that the workspace's MSRV doesn't get out-of-date anymore.

crepererum · 2024-12-18T13:02:19Z

I wonder if we should even have per-crate MSRVs or rather opt for a workspace-wide one. To me, the different ranges are close to impossible to keep track off.

tustvold · 2024-12-18T13:03:51Z

I wonder if we should even have per-crate MSRVs or rather opt for a workspace-wide one. To me, the different ranges are close to impossible to keep track off.

I think it would make sense for a single MSRV for arrow and a single MSRV for object_store (likely the same)

itsjunetime added 5 commits November 16, 2024 17:17

Update most MSRVs

96928c2

Make cargo-msrv verify every package in repo instead of just a select…

4ba624f

… few and purposefully break arrow-flight msrv

Add test to ensure workspace rust version is being used at least some…

25001d9

…where

Fix exit1 => exit 1

26498f0

Make arrow-flight work, at the very least, with 'cargo metadata'

5b0f602

github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate object-store Object Store Interface labels Nov 17, 2024

itsjunetime commented Nov 17, 2024

View reviewed changes

itsjunetime added 3 commits November 16, 2024 18:22

Fix arrow-flight/gen rust-version to make CI pass now

765c2f0

Get rid of pretty msrv logging as it can't all be displayed

f17f4cf

Do '-mindepth 2' with find to prevent running cargo msrv on the works…

4d6ffeb

…pace as a whole

alamb reviewed Nov 17, 2024

View reviewed changes

alamb added development-process Related to development process of arrow-rs and removed development-process Related to development process of arrow-rs labels Nov 17, 2024

alamb added 3 commits November 22, 2024 15:05

Use correct MSRV for object_store

bfe798d

remove workspace msrv check

3be0974

Merge remote-tracking branch 'apache/master' into june/update_msrvs

85806ee

alamb added the development-process Related to development process of arrow-rs label Nov 22, 2024

alamb previously approved these changes Nov 22, 2024

View reviewed changes

tustvold reviewed Nov 22, 2024

View reviewed changes

revert msrv

ddd66af

github-actions bot removed the object-store Object Store Interface label Nov 23, 2024

push object_store MSRV back down to 1.62.1

35e9f98

github-actions bot added the object-store Object Store Interface label Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MSRVs to be accurate #6742

Update MSRVs to be accurate #6742

itsjunetime commented Nov 17, 2024

itsjunetime Nov 17, 2024

itsjunetime commented Nov 17, 2024

alamb left a comment

alamb Nov 17, 2024

alamb Nov 17, 2024

alamb commented Nov 22, 2024

tustvold Nov 22, 2024

alamb Nov 23, 2024

itsjunetime Nov 25, 2024

alamb Nov 25, 2024

tustvold commented Nov 29, 2024

itsjunetime commented Dec 2, 2024

crepererum commented Dec 18, 2024

tustvold commented Dec 18, 2024

Update MSRVs to be accurate #6742

Are you sure you want to change the base?

Update MSRVs to be accurate #6742

Conversation

itsjunetime commented Nov 17, 2024

Which issue does this PR close?

What changes are included in this PR?

Rationale for this change

Are there any user-facing changes?

itsjunetime Nov 17, 2024

Choose a reason for hiding this comment

itsjunetime commented Nov 17, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 17, 2024

Choose a reason for hiding this comment

alamb Nov 17, 2024

Choose a reason for hiding this comment

alamb commented Nov 22, 2024

tustvold Nov 22, 2024

Choose a reason for hiding this comment

alamb Nov 23, 2024

Choose a reason for hiding this comment

itsjunetime Nov 25, 2024

Choose a reason for hiding this comment

alamb Nov 25, 2024

Choose a reason for hiding this comment

tustvold commented Nov 29, 2024

itsjunetime commented Dec 2, 2024

crepererum commented Dec 18, 2024

tustvold commented Dec 18, 2024